[feat] #192 Firebase Analytics(GA4) 이벤트 로깅 추가#195
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 40 minutes and 37 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
Walkthrough새로운 Changes
Sequence Diagram(s)sequenceDiagram
participant ViewModel as Archive ViewModel
participant Logger as AnalyticsLogger
participant FirebaseLogger as FirebaseAnalyticsLogger
participant Firebase as FirebaseAnalytics
participant Bundle as Android Bundle
ViewModel->>ViewModel: albumCreate() triggered
ViewModel->>Logger: log(ArchiveAnalyticsEvent.AlbumCreate)
Logger->>FirebaseLogger: log(event)
FirebaseLogger->>Bundle: Build Bundle from event.params
FirebaseLogger->>Firebase: logEvent("album_create", bundle)
Firebase-->>FirebaseLogger: Event recorded
FirebaseLogger-->>Logger: Complete
Logger-->>ViewModel: Complete
ViewModel->>ViewModel: Refresh folder list
sequenceDiagram
participant MapScreen as Map Screen
participant MapVM as MapViewModel
participant Logger as AnalyticsLogger
participant FirebaseLogger as FirebaseAnalyticsLogger
participant Firebase as FirebaseAnalytics
MapScreen->>MapVM: logMapView()
MapVM->>Logger: log(MapAnalyticsEvent.MapView)
Logger->>FirebaseLogger: log(event)
FirebaseLogger->>Firebase: logEvent("map_view", bundle)
MapScreen->>MapVM: ClickRefreshButton intent
MapVM->>MapVM: Calculate hasFilter & regionChanged
MapVM->>Logger: log(MapAnalyticsEvent.MapReSearch)
Logger->>FirebaseLogger: log(event)
FirebaseLogger->>Firebase: logEvent("map_re_search", {has_filter, region_changed})
MapVM->>MapVM: Perform search/fetch
sequenceDiagram
participant SplashVM as SplashViewModel
participant Logger as AnalyticsLogger
participant FirebaseLogger as FirebaseAnalyticsLogger
participant Firebase as FirebaseAnalytics
participant Auth as AuthRepository
SplashVM->>SplashVM: checkVersionAndAuth()
SplashVM->>Logger: setUserProperty("app_version", version)
Logger->>FirebaseLogger: setUserProperty()
FirebaseLogger->>Firebase: setUserProperty()
SplashVM->>Auth: updateAccessToken()
Auth-->>SplashVM: Success
SplashVM->>Logger: log(GlobalAnalyticsEvent.AppOpen)
Logger->>FirebaseLogger: log(event)
FirebaseLogger->>Firebase: logEvent("app_open", bundle)
SplashVM->>SplashVM: Navigate to Main
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
feature/auth/impl/src/main/kotlin/com/neki/android/feature/auth/impl/login/LoginViewModel.kt (1)
63-80:⚠️ Potential issue | 🔴 Critical수동 로그인 경로에서
app_open이벤트 로깅 누락.현재 코드에서는 자동 로그인 시(SplashViewModel.kt line 106)에만
analyticsLogger.log(GlobalAnalyticsEvent.AppOpen)이 기록되고 있으며, 수동 로그인(Kakao) 완료 후 Main으로 네비게이션하는 경로(line 71)에서는 이벤트가 기록되지 않고 있습니다. PR 설명의 "로그인/자동 로그인 완료 시점에 로깅"이라는 요구사항에 따르면, 두 로그인 경로에서 모두app_open이벤트를 기록해야 합니다.checkTermAgreementState()에서 Main으로 네비게이션할 때(또는 그 이후) AppOpen 로깅을 추가하시기 바랍니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/impl/src/main/kotlin/com/neki/android/feature/auth/impl/login/LoginViewModel.kt` around lines 63 - 80, In checkTermAgreementState(), ensure the app_open event is logged on the manual login path: after retrieving userInfo (userRepository.getUserInfo()) and setting analyticsLogger.setUserId(userInfo.id.toString()) and authRepository.setCompletedOnboarding(true), call analyticsLogger.log(GlobalAnalyticsEvent.AppOpen) before posting LoginSideEffect.NavigateToMain so the AppOpen event is emitted for the manual (Kakao) login flow; also add the same analyticsLogger.log(GlobalAnalyticsEvent.AppOpen) in the onFailure branch only if you still navigate to Main there (or keep it out if you always navigate to Term), referencing checkTermAgreementState, analyticsLogger, GlobalAnalyticsEvent.AppOpen, and LoginSideEffect.NavigateToMain.feature/map/impl/src/main/java/com/neki/android/feature/map/impl/MapViewModel.kt (1)
65-84:⚠️ Potential issue | 🟠 Major초기 검색 기준점이 없어 첫 재검색의
region_changed가 항상 false가 됩니다.
lastSearchCenter는 새로고침 이후에만 저장됩니다. 초기/자동 지도 로드 후 사용자가 멀리 이동해서 첫 재검색을 눌러도 Line 410에서 바로false가 반환되어map_re_search.region_changed가 실제 이동 여부를 반영하지 못합니다. 지도 영역을 로드할 때도 현재 검색 중심을 기준점으로 저장해 주세요.🛠️ 제안 변경 예시
is MapIntent.LoadPhotoBoothsByBounds -> { + lastSearchCenter = intent.mapBounds.center() loadPhotoBoothsByPolygon(intent.mapBounds, state, reduce, postSideEffect) }+private fun MapBounds.center(): LocLatLng = LocLatLng( + latitude = (northWest.latitude + southEast.latitude) / 2.0, + longitude = (northWest.longitude + southEast.longitude) / 2.0, +) + private fun isRegionChanged(currentCenter: LocLatLng, zoomLevel: Double): Boolean { val prev = lastSearchCenter ?: return falseAlso applies to: 409-419
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/map/impl/src/main/java/com/neki/android/feature/map/impl/MapViewModel.kt` around lines 65 - 84, The metric is wrong because lastSearchCenter is only set after manual refresh; update the initial load path so lastSearchCenter is initialized when the map first loads: in the MapIntent.LoadPhotoBoothsByBounds handling (and/or inside loadPhotoBoothsByPolygon), compute the current search center from intent.mapBounds (or from the same center calculation used by isRegionChanged) and assign it to lastSearchCenter before returning/dispatching the load; ensure the same assignment occurs for automatic/initial map load flows so isRegionChanged compares against a proper baseline.
🧹 Nitpick comments (2)
feature/map/impl/src/main/java/com/neki/android/feature/map/impl/MapContract.kt (1)
37-37:ClickRefreshButton에 파생 가능한 카메라 상태가 다수 포함됨 — 도메인 응집도 저하 우려.
mapBounds,center,zoomLevel이 모두CameraPosition에서 파생되는 값이라 Intent 페이로드가 비대해지고 추후 유지보수 시 세 값의 일관성을 호출부가 보장해야 합니다. 카메라 상태를 하나의 VO(예:CameraSnapshot(bounds, center, zoom))로 감싸 Intent에 싣는 리팩터가 테스트/확장성 측면에서 유리합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/map/impl/src/main/java/com/neki/android/feature/map/impl/MapContract.kt` at line 37, ClickRefreshButton currently carries three separate, derivable camera fields (mapBounds, center, zoomLevel) which reduces domain cohesion; create a single value object (e.g., CameraSnapshot) that encapsulates MapBounds, LocLatLng center and zoomLevel (or derive from CameraPosition) and replace the ClickRefreshButton payload to accept that VO instead (update the MapIntent definition, any factory/constructors that create ClickRefreshButton, and usages that read mapBounds/center/zoomLevel to use CameraSnapshot.getBounds()/getCenter()/getZoom()).feature/map/impl/src/main/java/com/neki/android/feature/map/impl/MapViewModel.kt (1)
196-211: Reducer 내부의 analytics side effect는 밖으로 빼는 편이 좋습니다.
reduce {}는 상태 변환만 담당하도록 두면 재실행/테스트/스토어 구현 변경 시 중복 로그 위험을 줄일 수 있습니다.updatedBrands와 이벤트 값을 reducer 밖에서 계산한 뒤 한 번만 log하도록 분리해 주세요.♻️ 리팩터링 방향 예시
-private fun handleClickBrand( - clickedBrand: Brand, - reduce: (MapState.() -> MapState) -> Unit, -) { - reduce { - val updatedBrands = brands.map { brand -> +private fun handleClickBrand( + clickedBrand: Brand, + state: MapState, + reduce: (MapState.() -> MapState) -> Unit, +) { + val updatedBrands = state.brands.map { brand -> + if (brand == clickedBrand) { + brand.copy(isChecked = !brand.isChecked) + } else { + brand + } + } + val checkedBrandNames = updatedBrands.filter { it.isChecked }.map { it.name } + + analyticsLogger.log( + MapAnalyticsEvent.MapBrandFilterToggle( + action = if (clickedBrand.isChecked) "deselect" else "select", + selectedCount = updatedBrands.count { it.isChecked }, + brandName = clickedBrand.name, + ), + ) + + reduce { + copy( + brands = updatedBrands.toImmutableList(), + mapMarkers = mapMarkers.map { photoBooth -> + photoBooth.copy( + isCheckedBrand = checkedBrandNames.isEmpty() || photoBooth.brandName in checkedBrandNames, + ) + }.toImmutableList(), + nearbyPhotoBooths = nearbyPhotoBooths.map { photoBooth -> + photoBooth.copy( + isCheckedBrand = checkedBrandNames.isEmpty() || photoBooth.brandName in checkedBrandNames, + ) + }.toImmutableList(), + ) + } - if (brand == clickedBrand) { - brand.copy(isChecked = !brand.isChecked) - } else { - brand - } - } - val checkedBrandNames = updatedBrands.filter { it.isChecked }.map { it.name } - analyticsLogger.log(...) - copy(...) - } }호출부도
handleClickBrand(intent.brand, state, reduce)로 맞춰야 합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/map/impl/src/main/java/com/neki/android/feature/map/impl/MapViewModel.kt` around lines 196 - 211, The analytics side effect inside the reduce block should be removed: compute updatedBrands and the MapAnalyticsEvent.MapBrandFilterToggle payload (including action, selectedCount, brandName) before calling reduce, then call reduce only to update state (using the updatedBrands) and immediately after call analyticsLogger.log once with the precomputed event; update the handler to use handleClickBrand(intent.brand, state, reduce) so the reducer does pure state transformation and logging happens externally (refer to symbols: reduce, updatedBrands, clickedBrand, analyticsLogger.log, MapAnalyticsEvent.MapBrandFilterToggle, handleClickBrand).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/analytics/src/main/kotlin/com/neki/android/core/analytics/event/AnalyticsEvent.kt`:
- Around line 3-8: Change AnalyticsEvent.params from Map<String, String> to
Map<String, Any> (update the sealed interface AnalyticsEvent accordingly) and
remove any .toString() uses in event implementations (e.g., totalSwipeCount,
count) so numeric values remain numeric; then update FirebaseAnalyticsLogger to
inspect each param value's runtime type and call the appropriate Firebase method
(putLong/putDouble/putString/etc.) for keys/values instead of always using
putString, handling nulls and booleans as appropriate.
In
`@core/analytics/src/main/kotlin/com/neki/android/core/analytics/logger/AnalyticsLogger.kt`:
- Around line 5-9: Add explicit clear/reset APIs to AnalyticsLogger so caller
can remove user id and properties instead of relying on nullable setters; update
the interface (AnalyticsLogger) to include either clearUserId() and
clearUserProperties() or a single reset() method, and implement these new
methods in the concrete analytics adapter to call
FirebaseAnalytics.resetAnalyticsData() (or equivalent) under the hood to
guarantee previous user identifiers/properties are cleared on logout/account
switch.
In
`@core/analytics/src/main/kotlin/com/neki/android/core/analytics/logger/FirebaseAnalyticsLogger.kt`:
- Around line 13-15: The Bundle construction in FirebaseAnalyticsLogger
currently forces all event params to strings; change the AnalyticsEvent.params
contract to Map<String, Any?> (or similar) and update the Bundle population in
FirebaseAnalyticsLogger (where you build the Bundle and call
FirebaseAnalytics.logEvent) to inspect each value and call the appropriate
Bundle.putString/putLong/putDouble (or skip nulls) so numeric counters like
count/album_count/selected_count/total_swipe_count are sent as long/double
rather than strings; ensure the updated param map type is reflected wherever
AnalyticsEvent.params is implemented.
In
`@feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/album_detail/AlbumDetailViewModel.kt`:
- Line 316: In AlbumDetailViewModel locate the success branch that currently
calls analyticsLogger.log(ArchiveAnalyticsEvent.PhotoCopy) (the import/use of
ArchiveAnalyticsEvent in the photo-add-from-detail success flow) and replace
that event with ArchiveAnalyticsEvent.AlbumAddFromDetail so the
import-from-detail flow is logged correctly; ensure any surrounding context (the
success handler or method in AlbumDetailViewModel that handles adding photos
from the bottom sheet) still compiles and that no other PhotoCopy occurrences
are unintentionally changed.
In
`@feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/main/ArchiveMainScreen.kt`:
- Around line 64-66: The current LaunchedEffect(Unit) in ArchiveMainScreen is
only run on first composition so viewModel.logArchivingView() won't fire when
returning to the tab; replace that LaunchedEffect usage with
LifecycleResumeEffect(Unit) so the effect runs on each lifecycle resume (tab
return) — update the call site in ArchiveMainScreen where LaunchedEffect(Unit)
wraps viewModel.logArchivingView() to use LifecycleResumeEffect(Unit) in the
same pattern used in MapScreen.kt.
In
`@feature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/PhotoDetailViewModel.kt`:
- Around line 205-210: In PhotoDetailViewModel.saveMemo (where
photoRepository.updateMemo is invoked), change analytics logging so
PhotoMemoCreate is emitted only for true creations (oldMemo.isBlank() &&
newMemo.isNotBlank()); for other cases (edit or delete) emit a different event
such as PhotoMemoSave or PhotoMemoUpdate via analyticsLogger.log, and keep
posting the existing side effect
(postSideEffect(PhotoDetailSideEffect.NotifyPhotoUpdated)).
In
`@feature/auth/impl/src/main/kotlin/com/neki/android/feature/auth/impl/login/LoginViewModel.kt`:
- Around line 66-68: 자동 로그인 경로과 약관 전환 시 User-ID가 누락되는 문제: 호출 지점인
SplashViewModel.fetchAuthState()의 자동 로그인 성공 분기와
LoginViewModel.checkTermAgreementState()에서 userRepository.getUserInfo() 실패 분기
모두에 analyticsLogger.setUserId(userInfo.id.toString())를 보장하도록 수정하세요; 구체적으로
LoginViewModel의 userRepository.getUserInfo().onSuccess {
analyticsLogger.setUserId(...) } 외에 자동 로그인 성공
콜백(SplashViewModel.fetchAuthState())에도 동일한 setUserId 호출을 추가하고,
LoginViewModel.checkTermAgreementState()의 getUserInfo 실패 경로에는 간단한 재시도(예: 1회 재조회)
로직을 넣어 재조회 후 성공 시 setUserId를 호출한 뒤 약관 화면으로 네비게이션하도록 구현하세요.
In
`@feature/map/impl/src/main/java/com/neki/android/feature/map/impl/MapScreen.kt`:
- Around line 71-73: LaunchedEffect(Unit) in MapScreen currently runs only once
at initial composition, so viewModel.logMapView() won't fire on tab return;
replace the LaunchedEffect(Unit) usage with a LifecycleResumeEffect (or
equivalent lifecycle-aware effect) that calls viewModel.logMapView() on each
onResume, ensuring the "_view" event is recorded every time the tab is resumed.
Locate MapScreen and replace the LaunchedEffect(Unit) block that invokes
logMapView() with a LifecycleResumeEffect calling viewModel.logMapView(); apply
the same change to the corresponding screens in feature/pose and feature/archive
where LaunchedEffect(Unit) is used.
In
`@feature/pose/impl/src/main/java/com/neki/android/feature/pose/impl/main/PoseScreen.kt`:
- Around line 61-63: LaunchedEffect(Unit) only runs on first composition so
viewModel.logPoseView() is recorded once; because the Pose tab is kept by
rememberSaveableStateHolderNavEntryDecorator you must trigger logging on each
tab return instead. Update the logic to depend on the current tab key (replace
LaunchedEffect(Unit) with LaunchedEffect(currentTabKey) or similar observable
that changes when the Pose tab becomes active) or move the call to the tab
selection handler so viewModel.logPoseView() is invoked whenever the Pose tab is
selected.
In
`@feature/pose/impl/src/main/java/com/neki/android/feature/pose/impl/random/RandomPoseViewModel.kt`:
- Around line 94-97: 현재 포즈가 없을 때도 이벤트가 기록되는 버그가 있으니
RandomPoseIntent.ClickBookmarkIcon 처리에서
analyticsLogger.log(PoseAnalyticsEvent.PoseBookmark)를 state.currentPose ?:
return 체크 뒤로 옮겨 실제 currentPose가 존재할 때만 호출하도록 변경하고, 기존의
handleBookmarkToggle(currentPost.id, !currentPost.isBookmarked, reduce) 호출 흐름은
그대로 유지하여 로그는 currentPost 변수(또는 state.currentPose) 조회 이후에만 기록되게 하세요.
---
Outside diff comments:
In
`@feature/auth/impl/src/main/kotlin/com/neki/android/feature/auth/impl/login/LoginViewModel.kt`:
- Around line 63-80: In checkTermAgreementState(), ensure the app_open event is
logged on the manual login path: after retrieving userInfo
(userRepository.getUserInfo()) and setting
analyticsLogger.setUserId(userInfo.id.toString()) and
authRepository.setCompletedOnboarding(true), call
analyticsLogger.log(GlobalAnalyticsEvent.AppOpen) before posting
LoginSideEffect.NavigateToMain so the AppOpen event is emitted for the manual
(Kakao) login flow; also add the same
analyticsLogger.log(GlobalAnalyticsEvent.AppOpen) in the onFailure branch only
if you still navigate to Main there (or keep it out if you always navigate to
Term), referencing checkTermAgreementState, analyticsLogger,
GlobalAnalyticsEvent.AppOpen, and LoginSideEffect.NavigateToMain.
In
`@feature/map/impl/src/main/java/com/neki/android/feature/map/impl/MapViewModel.kt`:
- Around line 65-84: The metric is wrong because lastSearchCenter is only set
after manual refresh; update the initial load path so lastSearchCenter is
initialized when the map first loads: in the MapIntent.LoadPhotoBoothsByBounds
handling (and/or inside loadPhotoBoothsByPolygon), compute the current search
center from intent.mapBounds (or from the same center calculation used by
isRegionChanged) and assign it to lastSearchCenter before returning/dispatching
the load; ensure the same assignment occurs for automatic/initial map load flows
so isRegionChanged compares against a proper baseline.
---
Nitpick comments:
In
`@feature/map/impl/src/main/java/com/neki/android/feature/map/impl/MapContract.kt`:
- Line 37: ClickRefreshButton currently carries three separate, derivable camera
fields (mapBounds, center, zoomLevel) which reduces domain cohesion; create a
single value object (e.g., CameraSnapshot) that encapsulates MapBounds,
LocLatLng center and zoomLevel (or derive from CameraPosition) and replace the
ClickRefreshButton payload to accept that VO instead (update the MapIntent
definition, any factory/constructors that create ClickRefreshButton, and usages
that read mapBounds/center/zoomLevel to use
CameraSnapshot.getBounds()/getCenter()/getZoom()).
In
`@feature/map/impl/src/main/java/com/neki/android/feature/map/impl/MapViewModel.kt`:
- Around line 196-211: The analytics side effect inside the reduce block should
be removed: compute updatedBrands and the MapAnalyticsEvent.MapBrandFilterToggle
payload (including action, selectedCount, brandName) before calling reduce, then
call reduce only to update state (using the updatedBrands) and immediately after
call analyticsLogger.log once with the precomputed event; update the handler to
use handleClickBrand(intent.brand, state, reduce) so the reducer does pure state
transformation and logging happens externally (refer to symbols: reduce,
updatedBrands, clickedBrand, analyticsLogger.log,
MapAnalyticsEvent.MapBrandFilterToggle, handleClickBrand).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 567d037a-d6cd-447d-9748-dd12683f1376
📒 Files selected for processing (31)
app/build.gradle.ktsapp/src/main/java/com/neki/android/app/MainActivity.ktbuild-logic/src/main/java/com/neki/android/buildlogic/plugins/AndroidFeatureImplConventionPlugin.ktcore/analytics/build.gradle.ktscore/analytics/src/main/kotlin/com/neki/android/core/analytics/event/AnalyticsEvent.ktcore/analytics/src/main/kotlin/com/neki/android/core/analytics/event/ArchiveAnalyticsEvent.ktcore/analytics/src/main/kotlin/com/neki/android/core/analytics/event/GlobalAnalyticsEvent.ktcore/analytics/src/main/kotlin/com/neki/android/core/analytics/event/MapAnalyticsEvent.ktcore/analytics/src/main/kotlin/com/neki/android/core/analytics/event/PoseAnalyticsEvent.ktcore/analytics/src/main/kotlin/com/neki/android/core/analytics/logger/AnalyticsLogger.ktcore/analytics/src/main/kotlin/com/neki/android/core/analytics/logger/AnalyticsModule.ktcore/analytics/src/main/kotlin/com/neki/android/core/analytics/logger/FirebaseAnalyticsLogger.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/album/AllAlbumViewModel.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/album_detail/AlbumDetailViewModel.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/main/ArchiveMainScreen.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/main/ArchiveMainViewModel.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/photo_detail/PhotoDetailViewModel.ktfeature/auth/impl/src/main/kotlin/com/neki/android/feature/auth/impl/login/LoginViewModel.ktfeature/auth/impl/src/main/kotlin/com/neki/android/feature/auth/impl/splash/SplashViewModel.ktfeature/map/impl/src/main/java/com/neki/android/feature/map/impl/MapContract.ktfeature/map/impl/src/main/java/com/neki/android/feature/map/impl/MapScreen.ktfeature/map/impl/src/main/java/com/neki/android/feature/map/impl/MapViewModel.ktfeature/map/impl/src/main/java/com/neki/android/feature/map/impl/const/MapConst.ktfeature/photo-upload/impl/build.gradle.ktsfeature/pose/impl/src/main/java/com/neki/android/feature/pose/impl/detail/PoseDetailViewModel.ktfeature/pose/impl/src/main/java/com/neki/android/feature/pose/impl/main/PoseScreen.ktfeature/pose/impl/src/main/java/com/neki/android/feature/pose/impl/main/PoseViewModel.ktfeature/pose/impl/src/main/java/com/neki/android/feature/pose/impl/random/RandomPoseViewModel.ktfeature/select-album/impl/src/main/java/com/neki/android/feature/select_album/impl/SelectAlbumViewModel.ktgradle/libs.versions.tomlsettings.gradle.kts
ikseong00
approved these changes
Apr 19, 2026
Contributor
ikseong00
left a comment
ikseong00
left a comment
There was a problem hiding this comment.
확인했습니다!
붙인 곳이 많은데 고생하셨습니다.
코멘트 확인해주세요!
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/map/impl/src/main/java/com/neki/android/feature/map/impl/MapViewModel.kt (1)
265-287:⚠️ Potential issue | 🟡 MinorMapRouteClick 로그가 가드 실패 경로에서도 남습니다.
현재
MapRouteClick은 함수 진입 직후 무조건 기록되지만, 그 뒤에서state.currentLocLatLng == null이면 토스트만 띄우고 리턴하고(L275-278), 포커스된 부스가 없으면LaunchDirectionApp효과가 실행되지 않습니다(L279-287). 결과적으로 실제로는 외부 지도 앱이 실행되지 않은 클릭도map_route_click으로 집계되어 지표가 부풀려질 수 있습니다. 로깅 시점을LaunchDirectionApp을 실제로 post하는 시점(또는 가드 통과 이후)으로 옮기는 것을 권장드립니다.♻️ 제안 변경
- analyticsLogger.log( - MapAnalyticsEvent.MapRouteClick( - mapType = when (app) { - DirectionApp.KAKAO_MAP -> "kakao_map" - DirectionApp.NAVER_MAP -> "naver_map" - DirectionApp.GOOGLE_MAP -> "google_map" - }, - ), - ) reduce { copy(isShowDirectionBottomSheet = false) } if (state.currentLocLatLng == null) { postSideEffect(MapEffect.ShowToastMessage("현재 위치를 가져올 수 없습니다.")) return } state.mapMarkers.find { it.isFocused }?.let { focusedPhotoBooth -> + analyticsLogger.log( + MapAnalyticsEvent.MapRouteClick( + mapType = when (app) { + DirectionApp.KAKAO_MAP -> "kakao_map" + DirectionApp.NAVER_MAP -> "naver_map" + DirectionApp.GOOGLE_MAP -> "google_map" + }, + ), + ) postSideEffect( MapEffect.LaunchDirectionApp( app = app, startLocLatLng = state.currentLocLatLng, endLocLatLng = LocLatLng(focusedPhotoBooth.latitude, focusedPhotoBooth.longitude), ), ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/map/impl/src/main/java/com/neki/android/feature/map/impl/MapViewModel.kt` around lines 265 - 287, Move the MapRouteClick analytics log so it only records when the direction flow actually proceeds: after checking state.currentLocLatLng != null and after finding a focused marker, just before calling postSideEffect(MapEffect.LaunchDirectionApp(...)). Specifically, remove the early analyticsLogger.log(MapAnalyticsEvent.MapRouteClick(...)) at the top of the function and insert the same analyticsLogger.log call immediately prior to postSideEffect(MapEffect.LaunchDirectionApp(...)) (i.e., after the null guard on state.currentLocLatLng and inside the mapMarkers.find { it.isFocused }?.let { ... } block), leaving other behavior like reduce { copy(isShowDirectionBottomSheet = false) } and the toast post on guard failure unchanged.
🧹 Nitpick comments (1)
feature/map/impl/src/main/java/com/neki/android/feature/map/impl/MapViewModel.kt (1)
410-420: 최초 재검색 시region_changed의미 확인 및 임계값 상수화 제안.
lastSearchCenter가 null인 첫 재검색에서는 항상false가 반환됩니다. GA4 이벤트 스펙상 "처음 재검색은 region_changed=false"로 해석해도 되는지, 혹은 "이전 검색이 없으면 이벤트 자체를 생략/true 처리"해야 하는지 노션 이벤트 설계 문서와 한 번 맞춰봐 주세요.- 300/500/700/1000, 18/16/14 같은 상수는 PR 설명에 언급된 줌 임계값과 일치하지만, 현재는 매직 넘버로 흩어져 있어 향후 튜닝 시 누락되기 쉽습니다.
MapConst또는 이 ViewModel 상단의companion object로 추출하면 조정·테스트가 수월해집니다.♻️ 참고용 리팩터링 예시
+ companion object { + private const val REGION_CHANGE_THRESHOLD_ZOOM_18 = 300 + private const val REGION_CHANGE_THRESHOLD_ZOOM_16 = 500 + private const val REGION_CHANGE_THRESHOLD_ZOOM_14 = 700 + private const val REGION_CHANGE_THRESHOLD_DEFAULT = 1000 + } + private fun isRegionChanged(currentCenter: LocLatLng, zoomLevel: Double): Boolean { val prev = lastSearchCenter ?: return false val distance = calculateDistance(prev.latitude, prev.longitude, currentCenter.latitude, currentCenter.longitude) val threshold = when { - zoomLevel >= 18 -> 300 - zoomLevel >= 16 -> 500 - zoomLevel >= 14 -> 700 - else -> 1000 + zoomLevel >= 18 -> REGION_CHANGE_THRESHOLD_ZOOM_18 + zoomLevel >= 16 -> REGION_CHANGE_THRESHOLD_ZOOM_16 + zoomLevel >= 14 -> REGION_CHANGE_THRESHOLD_ZOOM_14 + else -> REGION_CHANGE_THRESHOLD_DEFAULT } return distance >= threshold }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/map/impl/src/main/java/com/neki/android/feature/map/impl/MapViewModel.kt` around lines 410 - 420, The isRegionChanged function returns false when lastSearchCenter is null and uses magic numbers for zoom thresholds and distances; confirm with the GA4/notion event spec whether the initial search with no previous center should emit region_changed=false, be omitted, or treated as true, then update isRegionChanged (and any event emission call sites) to follow that spec instead of defaulting to false; extract the numeric thresholds (18,16,14 and 300,500,700,1000) into named constants (either a MapConst object or the ViewModel's companion object) and replace the inline literals with those constants so future tuning is centralized (reference symbols: isRegionChanged, lastSearchCenter, calculateDistance).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@feature/map/impl/src/main/java/com/neki/android/feature/map/impl/MapViewModel.kt`:
- Around line 265-287: Move the MapRouteClick analytics log so it only records
when the direction flow actually proceeds: after checking state.currentLocLatLng
!= null and after finding a focused marker, just before calling
postSideEffect(MapEffect.LaunchDirectionApp(...)). Specifically, remove the
early analyticsLogger.log(MapAnalyticsEvent.MapRouteClick(...)) at the top of
the function and insert the same analyticsLogger.log call immediately prior to
postSideEffect(MapEffect.LaunchDirectionApp(...)) (i.e., after the null guard on
state.currentLocLatLng and inside the mapMarkers.find { it.isFocused }?.let {
... } block), leaving other behavior like reduce {
copy(isShowDirectionBottomSheet = false) } and the toast post on guard failure
unchanged.
---
Nitpick comments:
In
`@feature/map/impl/src/main/java/com/neki/android/feature/map/impl/MapViewModel.kt`:
- Around line 410-420: The isRegionChanged function returns false when
lastSearchCenter is null and uses magic numbers for zoom thresholds and
distances; confirm with the GA4/notion event spec whether the initial search
with no previous center should emit region_changed=false, be omitted, or treated
as true, then update isRegionChanged (and any event emission call sites) to
follow that spec instead of defaulting to false; extract the numeric thresholds
(18,16,14 and 300,500,700,1000) into named constants (either a MapConst object
or the ViewModel's companion object) and replace the inline literals with those
constants so future tuning is centralized (reference symbols: isRegionChanged,
lastSearchCenter, calculateDistance).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e9820283-b0ad-4453-84c2-0cf4f2431b7f
📒 Files selected for processing (6)
core/analytics/src/main/kotlin/com/neki/android/core/analytics/event/AnalyticsEvent.ktcore/analytics/src/main/kotlin/com/neki/android/core/analytics/event/ArchiveAnalyticsEvent.ktcore/analytics/src/main/kotlin/com/neki/android/core/analytics/event/MapAnalyticsEvent.ktcore/analytics/src/main/kotlin/com/neki/android/core/analytics/event/PoseAnalyticsEvent.ktcore/analytics/src/main/kotlin/com/neki/android/core/analytics/logger/FirebaseAnalyticsLogger.ktfeature/map/impl/src/main/java/com/neki/android/feature/map/impl/MapViewModel.kt
✅ Files skipped from review due to trivial changes (2)
- core/analytics/src/main/kotlin/com/neki/android/core/analytics/logger/FirebaseAnalyticsLogger.kt
- core/analytics/src/main/kotlin/com/neki/android/core/analytics/event/MapAnalyticsEvent.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- core/analytics/src/main/kotlin/com/neki/android/core/analytics/event/AnalyticsEvent.kt
- core/analytics/src/main/kotlin/com/neki/android/core/analytics/event/PoseAnalyticsEvent.kt
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
feature/select-album/impl/src/main/java/com/neki/android/feature/select_album/impl/SelectAlbumViewModel.kt (1)
133-147: 중첩when (action)단순화 제안 (선택).외부
when의is UploadFromQR, is UploadFromGallery ->결합 브랜치 안에서method문자열을 얻기 위해 다시when (action)으로 분기하고 있습니다. 두 케이스를 외부에서 분리하거나 로깅/공통 side effect를 작은 헬퍼로 추출하면 가독성이 좀 더 좋아집니다. 현재도 기능상 문제는 없습니다.♻️ 예시 리팩터
- is SelectAlbumAction.UploadFromQR, is SelectAlbumAction.UploadFromGallery -> { - analyticsLogger.log( - ArchiveAnalyticsEvent.PhotoUpload( - method = when (action) { - is SelectAlbumAction.UploadFromQR -> "qr" - is SelectAlbumAction.UploadFromGallery -> "gallery" - }, - count = photoCount, - ), - ) - - postSideEffect(SelectAlbumSideEffect.ShowToastMessage("이미지를 추가했어요")) - postSideEffect(SelectAlbumSideEffect.SendUploadResult(albums.first())) - } + is SelectAlbumAction.UploadFromQR -> handleUploadSuccess("qr", albums, postSideEffect) + is SelectAlbumAction.UploadFromGallery -> handleUploadSuccess("gallery", albums, postSideEffect)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/select-album/impl/src/main/java/com/neki/android/feature/select_album/impl/SelectAlbumViewModel.kt` around lines 133 - 147, The nested when can be simplified: inside the branch handling SelectAlbumAction.UploadFromQR and SelectAlbumAction.UploadFromGallery avoid the inner when(action) by deriving the method string once (e.g., val method = if (action is SelectAlbumAction.UploadFromQR) "qr" else "gallery") or split into two separate when branches (one for UploadFromQR, one for UploadFromGallery); then call analyticsLogger.log(ArchiveAnalyticsEvent.PhotoUpload(method = method, count = photoCount)) and the shared postSideEffect calls (SelectAlbumSideEffect.ShowToastMessage and SendUploadResult(albums.first())) without repeating the nested when. This refactor targets the SelectAlbumAction.UploadFromQR/UploadFromGallery branch, ArchiveAnalyticsEvent.PhotoUpload, analyticsLogger.log, and the postSideEffect calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/mypage/impl/src/main/java/com/neki/android/feature/mypage/impl/main/MyPageViewModel.kt`:
- Around line 199-200: The withdraw analytics event is being logged with
analyticsLogger.log(MypageAnalyticsEvent.Withdraw) immediately followed by
analyticsLogger.resetAnalytics(), which can clear pending events before they
reach DebugView/GA4; move the resetAnalytics() call out of the immediate path so
the Withdraw event is allowed to be delivered (e.g., delay/reset after confirmed
delivery or app restart), or introduce/use a separate API that only clears user
identity (e.g., clearUserId/clearIdentity) instead of resetAnalytics(); update
the code around analyticsLogger.log and analyticsLogger.resetAnalytics()
accordingly to ensure the Withdraw event is preserved.
- Around line 187-189: In the logout() flow, after logging the logout event with
analyticsLogger.log(MypageAnalyticsEvent.Logout) and before clearing tokens with
tokenRepository.clearTokensWithAuthCache(), call
analyticsLogger.resetAnalytics() to clear user identification; follow the same
pattern used in withdrawAccount() where analyticsLogger.resetAnalytics() is
invoked prior to token/cache clearing so subsequent events are not attributed to
the previous user.
---
Nitpick comments:
In
`@feature/select-album/impl/src/main/java/com/neki/android/feature/select_album/impl/SelectAlbumViewModel.kt`:
- Around line 133-147: The nested when can be simplified: inside the branch
handling SelectAlbumAction.UploadFromQR and SelectAlbumAction.UploadFromGallery
avoid the inner when(action) by deriving the method string once (e.g., val
method = if (action is SelectAlbumAction.UploadFromQR) "qr" else "gallery") or
split into two separate when branches (one for UploadFromQR, one for
UploadFromGallery); then call
analyticsLogger.log(ArchiveAnalyticsEvent.PhotoUpload(method = method, count =
photoCount)) and the shared postSideEffect calls
(SelectAlbumSideEffect.ShowToastMessage and SendUploadResult(albums.first()))
without repeating the nested when. This refactor targets the
SelectAlbumAction.UploadFromQR/UploadFromGallery branch,
ArchiveAnalyticsEvent.PhotoUpload, analyticsLogger.log, and the postSideEffect
calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f42c3fa0-7a64-435b-b0c2-bf4bd769e543
📒 Files selected for processing (10)
core/analytics/src/main/kotlin/com/neki/android/core/analytics/event/MypageAnalyticsEvent.ktcore/analytics/src/main/kotlin/com/neki/android/core/analytics/logger/AnalyticsLogger.ktcore/analytics/src/main/kotlin/com/neki/android/core/analytics/logger/FirebaseAnalyticsLogger.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/album_detail/AlbumDetailViewModel.ktfeature/archive/impl/src/main/kotlin/com/neki/android/feature/archive/impl/navigation/ArchiveEntryProvider.ktfeature/auth/impl/src/main/kotlin/com/neki/android/feature/auth/impl/login/LoginViewModel.ktfeature/auth/impl/src/main/kotlin/com/neki/android/feature/auth/impl/splash/SplashViewModel.ktfeature/mypage/impl/src/main/java/com/neki/android/feature/mypage/impl/main/MyPageViewModel.ktfeature/select-album/api/src/main/java/com/neki/android/feature/select_album/api/SelectAlbumAction.ktfeature/select-album/impl/src/main/java/com/neki/android/feature/select_album/impl/SelectAlbumViewModel.kt
✅ Files skipped from review due to trivial changes (1)
- core/analytics/src/main/kotlin/com/neki/android/core/analytics/event/MypageAnalyticsEvent.kt
🚧 Files skipped from review as they are similar to previous changes (4)
- feature/auth/impl/src/main/kotlin/com/neki/android/feature/auth/impl/login/LoginViewModel.kt
- core/analytics/src/main/kotlin/com/neki/android/core/analytics/logger/AnalyticsLogger.kt
- feature/auth/impl/src/main/kotlin/com/neki/android/feature/auth/impl/splash/SplashViewModel.kt
- core/analytics/src/main/kotlin/com/neki/android/core/analytics/logger/FirebaseAnalyticsLogger.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 관련 이슈
📙 작업 설명
core:analytics모듈 생성 및 Firebase Analytics 로깅 인터페이스 구현ArchiveAnalyticsEvent,PoseAnalyticsEvent,MapAnalyticsEvent,GlobalAnalyticsEvent)20.0, 기본 줌 레벨14.0변경AndroidFeatureImplConventionPlugin에core:analytics공통 의존성 추가🧪 테스트 내역
📸 스크린샷 또는 시연 영상
💬 추가 설명 or 리뷰 포인트
CameraX와Guava의존성 충돌 발생 →feature:photo-upload모듈에guava의존성 명시적으로 추가하여 해결했습니다.Summary by CodeRabbit
릴리스 노트
새로운 기능
개선사항